-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Switch block labels to natural language #231
Conversation
Eg. use "Remove" instead of "Queue Free". Also, remove the "On physics process" block. This is confusing since for the default project settings the _process runs at the same pace. https://phabricator.endlessm.com/T35563
The labels have changed "body" by "something" so the blocks holding the "body" variable have dissapeared.
97af8ba
to
e383ce9
Compare
@@ -10,8 +10,8 @@ description = "" | |||
category = "Communication | Methods" | |||
type = 1 | |||
variant_type = 0 | |||
display_template = "On [body: OBJECT] entered" | |||
code_template = "func _on_body_entered(body: Node2D): | |||
display_template = "When entering collision with [something: OBJECT]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the "entering collision with" / "leaving collision with" pair. I don't think collisions are something you leave. It's awkward to phrase this because we don't have a subject
Brainstorming:
- When colliding with [something] / When no longer colliding with [something]
- When this node collides with [something] / When this node stops colliding with [something]
- When collision with [something] starts / When collision with [something] ends
- When [something] collides with this node / When [something] stops colliding with this node
Not sure I like these much better…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the second one of those; "When this node collides with / stops colliding with [something]". Alternatively, we could write these in the first person, like "When I collide with", which I think is the nicest to read here but stylistically it's tricky because we don't want to be writing everything in first person.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point, should we switch everything to first person? Ie. "Remove me" instead of "Remove"? I'm open to do it.
description = "Attached blocks will be executed during the processing step of the main loop" | ||
category = "Lifecycle" | ||
type = 1 | ||
variant_type = 0 | ||
display_template = "On Process" | ||
display_template = "Every frame" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we going to consistently use Sentence case rather than Title case for blocks?
I checked what MakeCode Arcade does and actually it uses lower case!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I proposed lower case once to the other Will. Today we have a mix of Title case for short labels ("Viewport Center") and Sentence case for long labels. I just checked and Scratch has all in lower case too. So unless anyone has a strong argument I'll switch all to lower case.
- The display template in resource files. - The display template in blocks defined dynamically for properties. - The display template in blocks for the simple nodes. - The true and false booleans. - The options in dropdowns. https://phabricator.endlessm.com/T35563
Ready for review again. I think that the switch to lower case makes the label much more readable inside blocks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be fine but now all the block names are in the diff it's easier to review the others :)
@@ -10,7 +10,7 @@ description = "Generate a random floating point number between [i]from[/i] and [ | |||
category = "Math" | |||
type = 3 | |||
variant_type = 3 | |||
display_template = "Random floating point number between {from: FLOAT} and {to: FLOAT}" | |||
display_template = "random floating point number between {from: FLOAT} and {to: FLOAT}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A question for another day, but, do students know what "floating point number" means? Would "real number" be better understood?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we should remove one of these random number blocks and have a single "random number between X and Y". What I'm not sure about is which one should survive, the one for floats or the one for ints. Let's do that as follow-up.
If/when we're looking at English language optimization can we check the animation player blocks text please (I think it had something like "play animation ahead" which can maybe be improved). |
Yes, is "play animation forward" better? |
Co-authored-by: Will Thompson <[email protected]>
@wjt @dylanmccall this is ready for review again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Eg. use "Remove" instead of "Queue Free".
Also, remove the "On physics process" block. This is confusing since for the default project settings the _process runs at the same pace.
https://phabricator.endlessm.com/T35563